-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ext/node): http2session ready state #25143
Conversation
Signed-off-by: Caleb Lloyd <caleblloyd@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks for your contribution @caleblloyd!
Fixes denoland#25142 Signed-off-by: Caleb Lloyd <caleblloyd@gmail.com> (cherry picked from commit 79c7db3)
@satyarohith Should we add a test for this? |
Sure. I verified that the flag is set correctly but didn't ask for a test to avoid bloating the suite. |
We can never have enough tests. The more tests we have, the easier it will be to do refactorings in the future as it allows us to be more confident that we didn't break something accidentally in the process. |
If you can point me to the right file to write a test in, I can attempt to add one! |
Looks like https://github.com/denoland/deno/blob/main/tests/unit_node/http2_test.ts is the right file, let me try to get one in there... |
Fixes #25142